Failing test seems related. e.g.

2025-12-29T07:08:54.4550616Z 1) Test\Files\ViewTest::testCopyBetweenStorageNoCross
2025-12-29T07:08:54.4551112Z BadMethodCallException: path needs to be relative to the system wide data folder and point to a user specific file
2025-12-29T07:08:54.4552424Z 
2025-12-29T07:08:54.4552642Z /home/runner/actions-runner/_work/server/server/lib/private/Encryption/Util.php:208
2025-12-29T07:08:54.4553192Z /home/runner/actions-runner/_work/server/server/lib/private/Encryption/Keys/Storage.php:418
2025-12-29T07:08:54.4553758Z /home/runner/actions-runner/_work/server/server/lib/private/Encryption/Keys/Storage.php:368
2025-12-29T07:08:54.4554377Z /home/runner/actions-runner/_work/server/server/lib/private/Files/Storage/Wrapper/Encryption.php:877
2025-12-29T07:08:54.4555029Z /home/runner/actions-runner/_work/server/server/lib/private/Files/Storage/Wrapper/Encryption.php:673
2025-12-29T07:08:54.4555559Z /home/runner/actions-runner/_work/server/server/lib/private/Files/Storage/Wrapper/Encryption.php:582
2025-12-29T07:08:54.4556062Z /home/runner/actions-runner/_work/server/server/lib/private/Files/Storage/Wrapper/Wrapper.php:289
2025-12-29T07:08:54.4556501Z /home/runner/actions-runner/_work/server/server/lib/private/Files/View.php:968
2025-12-29T07:08:54.4556878Z /home/runner/actions-runner/_work/server/server/tests/lib/Files/ViewTest.php:475
2025-12-29T07:08:54.4557827Z /home/runner/actions-runner/_work/server/server/tests/lib/Files/ViewTest.php:454

Impact: When S3 is configured as primary object storage (via objectstore in config.php), it mounts at /, so encryption was NEVER applied, regardless of settings.

This is not true, the home storage is mounted at /$user same as with local storage.

Working through propagating getBoolVal everywhere.

Your tests fail bc the IAppConfig methods are called before the Nextcloud install is finished. The old code handled that gracefully, but the current code does not. Please change your code to wrap the IAppConfig calls in Manager::isEnabled() and EncryptionWrapper::wrapStorage() with a try/catch for \OCP\DB\Exception (or \Throwable) that returns the safe default, similar to how the old IConfig path behaved.

@miaulalala Okay, I'm back!

Did the things to convert to a boolean throughout for that app config setting.

@miaulalala Okay, I'm back!

Did the things to convert to a boolean throughout for that app config setting.

PR is looking good, tests pass. @artonge or @CarlSchwan can either of you review?

Thanks for the submission @cuppett.

Given the sensitive nature of the code, would it be possible for you to submit the change in several easier to review pull requests?

@artonge A bulk of the changes are in testing. The logic changed by +222 / -65. The rest is testing. Teasing that apart will be challenging logically. I think I could rearrange individual commits depending on how you want to review it.

Pushed through rebase to newest master

Rebase only.

Teasing that apart will be challenging logically.

Your summary in your first comment suggests that you identified key changes, that would probably be a good way to split it.

I think I could rearrange individual commits depending on how you want to review it.

Indeed, 18 commits seem like a lot compared to your summary.

Maybe to give a bit of context, we are receiving an increasing number of large PRs, which takes a lot of time to review, splitting the PR into manageable chunks would drastically help us to review it. Some can probably be quickly reviewed and merged, other might require some discussion.

Teasing that apart will be challenging logically.

Your summary in your first comment suggests that you identified key changes, that would probably be a good way to split it.

I think I could rearrange individual commits depending on how you want to review it.

Indeed, 18 commits seem like a lot compared to your summary.

Maybe to give a bit of context, we are receiving an increasing number of large PRs, which takes a lot of time to review, splitting the PR into manageable chunks would drastically help us to review it. Some can probably be quickly reviewed and merged, other might require some discussion.

Let me squash them into the 5 passes. Waiting 5 months to get these first couple reviews (or peeks) isn't something I want to repeat. :)

@artonge I got it squashed to 5 discrete commits. Additional commits were mostly fix/patches for tests. Combined no 1 and 2 from summary, but 5th commit is the pervasive IAppConfig bool usage throughout and fixups. Waiting on tests now. Thanks!

Related Issue Analysis

Reviewed open issues related to S3 encryption and zero-byte file problems. This PR addresses or is relevant to the following:

Fully Fixed

#58778 — conflict between new type (mixed) and old type (boolean) on re-encrypt

After occ encryption:decrypt-all followed by occ encryption:encrypt-all, users hit conflict between new type (mixed) and old type (boolean) in AppConfig.php and must manually delete the core/encryption_enabled row from oc_appconfig to recover.

Root cause: Disable.php wrote encryption_enabled as string 'no', but IAppConfig internally stored it as bool 0. Enable.php then tried to write string 'yes', triggering a type conflict.

How this PR fixes it: All Enable/Disable/DecryptAll commands now use setValueBool()/getValueBool() instead of untyped setAppValue(). The RetypeEncryptionConfigKeys repair step converts any existing string-typed values to bool on upgrade, eliminating type conflicts entirely.

Partially Fixed / Directly Related

#41992 — Server-side encryption does not encrypt files with S3 primary storage

Files upload to S3 but are NOT encrypted (only very small text files encrypt, larger files remain unencrypted).

What this PR fixes: The EncryptionWrapper conditional flow is refactored into clear early-return guards. The encryptHomeStorage check is moved from deep inside shouldEncrypt() during per-file fopen() to wrapper-application time. When encryption IS enabled for home storage, the wrapper is now consistently applied with correct parameters. This makes wrapper application deterministic, which likely resolves cases where the wrapper was inconsistently applied.

What remains: If the encryption module's shouldEncrypt() has separate bugs for multipart/chunked uploads on S3, those are outside this PR's scope. The reported symptom (small files encrypt, large files don't) could suggest a multipart-specific path issue deeper than wrapper application.

#59635 — Config for files_external mount option "encryption" is not respected

External S3 mount with "Activate Encryption" = false still encrypts files when SSE is globally enabled.

What this PR fixes: The EncryptionWrapper now has a clear guard-chain pattern with early returns. The IDisableEncryptionStorage check runs early and cleanly, and HomeMountPoint + encryptHomeStorage is explicitly handled.

What remains: The per-mount "Activate Encryption" toggle for files_external needs to either implement IDisableEncryptionStorage or be checked in the wrapper's guard chain, which this PR does not add. The guard-chain pattern introduced here makes adding that check straightforward in a follow-up.

#58239 — Regenerating metadata with SSE serves files encrypted (not decrypted)

After occ files:scan --all --generate-metadata, files are served encrypted instead of being decrypted.

What this PR fixes: The Scanner now correctly preserves unencrypted_size for encrypted files during rescans. Previously, the scanner stripped unencrypted_size from scan data unconditionally, potentially causing the encryption wrapper to mishandle files on subsequent reads.

What remains: If the metadata scan resets the encrypted flag itself or corrupts encryption key references, that's a separate code path. Testing with the specific reproduction steps from the issue would confirm whether this PR resolves it.

Fixes #58778

S3 Encryption Verification — Real AWS S3 Bucket

Ran the full S3 encryption test suite against a real AWS S3 bucket (us-east-1) to verify the fix for #41992 ("Server side encryption does not encrypt files with S3 primary storage").

Setup

• Fresh Nextcloud install with SQLite + S3 primary storage
• Server-side encryption enabled with master key
• PHPUnit test suite exercising the actual encryption/decryption pipeline through real S3

Results

PHPUnit 11.5.50 — Tests: 20, Assertions: 106, Skipped: 1 (110MB multipart gated)
OK

110MB multipart (run separately): Tests: 1, Assertions: 6
OK

Addressing #41992

The core symptom in #41992 was: "Files will sync and upload to the S3 provider but are not encrypted, unless they are very small." — specifically 8MB+ files remained unencrypted.

The testSizeConsistencyAcrossSources test explicitly asserts that the S3 object size > original file size for every test size (5MB, 16MB), proving encryption overhead is present. The 110MB multipart test confirms the same for chunked uploads that exceed the ~100MB multipart threshold.

Three assertions prove encryption is working end-to-end:

1. S3 object is larger than the original (encryption header + padding overhead)
2. DB unencrypted_size equals original file size (correct metadata tracking)
3. Downloaded content matches original (decryption pipeline works)

This branch fully addresses #41992 for all file sizes including multipart uploads.

Analysis: #59635 — Per-mount encryption toggle not respected

Result: Not fixed by this branch, but root cause identified

The per-mount "Activate Encryption" toggle failing is a separate bug in apps/files_external/lib/Service/DBConfigService.php, unrelated to this PR's changes.

Root cause

DBConfigService::setOption() at line 387 has a string $value type hint:

public function setOption(int $mountId, string $key, string $value): void {
    // ...
    ->setValue('value', $builder->createNamedParameter(json_encode($value), ...))

When called with false (boolean) from StoragesService::updateStorage():

foreach ($changedOptions as $key => $value) {
    $this->dbConfig->setOption($id, $key, $value);  // $value is bool false
}

PHP silently coerces false → "" due to the string type hint. Then json_encode("") = '""' is stored in oc_external_options. On read, json_decode('""') = "" (empty string). The shouldEncrypt() check at Encryption.php:916 is === false, which never matches an empty string — so encryption proceeds.

Traced end-to-end:

What would fix it: Change setOption's parameter type from string to mixed, so json_encode(false) stores false (the JSON boolean) instead of "".

Verification

Confirmed against a live Nextcloud 34 install with S3 as primary storage:

sqlite> SELECT key, value FROM oc_external_options WHERE mount_id=1;
enable_sharing|""
encrypt|""

After occ files_external:option 1 encrypt false, the DB stores "" instead of false. The per-mount toggle is silently ignored at runtime.

Status

This PR's EncryptionWrapper refactoring makes the guard chain cleaner and extensible, but the per-mount encrypt option was always handled downstream in shouldEncrypt() (unchanged here). The fix belongs in DBConfigService::setOption() and is worth a separate issue/PR targeting files_external.

@cuppett please, cleanup that noise, this is the opposite of what would be respectful to us. I assume you did not even took the time to double-check those outputs, why would we? I am asking you again to make it easy for us to review your changes, split your PR, give us consise summaries that go straight to the point, not AI generated rivers of text.

@cuppett please, cleanup that noise, this is the opposite of what would be respectful to us. I assume you did not even took the time to double-check those outputs, why would we? I am asking you again to make it easy for us to review your changes, split your PR, give us consise summaries that go straight to the point, not AI generated rivers of text.

Okay, I'll sequence them and pop the refs in here for lineage. Easy enough now with the squashed ones to land in a good order.